Lotsastuff: My earlier changes, BenBE's changes, julthomas' fixes #24

Merged
merged 27 commits into from Jul 15, 2011

Conversation

Projects
None yet
2 participants
@mpokrywka
Collaborator

mpokrywka commented Feb 28, 2011

Tested locally on my development machine, I will notify if my branch is production tested.

I have few shell scripts to test complied module - custom apache config to run as unprivileged user + shell scripts using CURL to upload files. When I clean them up, they could be attached as something resembling testing suite (though I'm not tdd believer).

When you (drogus, BenBE) have q's about commits, use github's commit comments or privmsg, but I may not be available to answer immediately.

About tagged release, I thought about one more change: add more info to upload_progress_cache_t (this struct will be on the beginning of shared mem). With additional info (node_t size, shared mem size, struct version?, parseable node_t definition? - fields offsets and sizes) robust tool for querying cache could be created

mpokrywka and others added some commits Feb 3, 2011

Reduce code indentation, move common code extracting per_dir_config t…
…o function

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Add debugging info when lock file is removed
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Do not create private pool, use global
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Use apache module infrastructure to merge vhosts configs
Side effect - all vhosts configs now point to same struct. Progress nodes
memory is shared between vhosts, so same config can also be shared.

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Remove "server" field from ServerConfig
Now server config is shared, use appropriate server for logging
instead of config->server.
Also assign global_server earlier (during create_server_config).

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Cosmetic code changes (removed unused local variables, renamed local …
…variables)

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Do not store pointers in shared memory
Keeping pointers to nodes and active list in shared memory was my bad idea.
Although it works, because child processes inherit shared memory mapped at the
same address as parent that allocated aformentioned shmem, this excludes usage
of shmem by other processes (ie. prospective upload progress cache browser)
Shared memory block is "partitioned" manually without use of apache rmm interface
(pointer arithmetic is easy ;-)

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Regenerate shared memory pointers in child processes
Inspired by mod_shm_counter

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Implemented proper locking on request cleanup function
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Use proper server for debug logging in track_upload_progress
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Add debugging for very unlikely condition in track_upload_progress
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Add more debugging noise (useful for debugging apache prefork simulta…
…neous requests)

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Log cache locking failure to proper server error log
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
"expires" node field changed to "updated_at", updated related logic
Expire timeout is now configurable

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Implemented "ETA" and "download completed time" features
Inspired by BenBE's a033210 but doesn't need additional
node field.

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Make all functions static to restrict their visibility
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Rename file containing backported function
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Backport ap_is_HTTP_VALID_RESPONSE macro if not present
Based on commit ee7679f

Author: Julien Thomas <julthomas@free.fr>
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Fix file size types and related format strings
Based on commit ee7679f

Author: Julien Thomas <julthomas@free.fr>
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Report non-POST requests when extensive debugging is enabled.
Author: BenBE <BenBE@geshi.org>
Signed-off-by: BenBE <BenBE@geshi.org>
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Updated README
Signed-off-by: BenBE <BenBE@geshi.org>
Change argument type of find_node function
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Remove check_node function
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Increase performance by taking less global locks
Store progress data in request local pool, update shared memory twice per second

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Remove obsolete forward declaration
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Remove fill_new_upload_data function
Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
Move cache data writing to separate function
In future this function may implement writing to remote cache (memcached/redis/etc)

Signed-off-by: Michał Pokrywka <michal.pokrywka@gmail.com>
@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

Potentially might allow for racing condition with multiple uploads by different source connections due to started at rule. Shouldn't it check if its really the same connection in such a case?

Potentially might allow for racing condition with multiple uploads by different source connections due to started at rule. Shouldn't it check if its really the same connection in such a case?

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 18, 2011

Owner

When two requests with same progress-id start at same second (equal "started_at"),
upload progress cache would be filled alternately with data from both requests.
I assumed such occurence is highly unlikely, so I used started_at as "connection identifier"
and no additional shared memory was used.
There could be additional field with randomly generated connection id,
but I think it's not worth the hassle.

Owner

mpokrywka replied Jul 18, 2011

When two requests with same progress-id start at same second (equal "started_at"),
upload progress cache would be filled alternately with data from both requests.
I assumed such occurence is highly unlikely, so I used started_at as "connection identifier"
and no additional shared memory was used.
There could be additional field with randomly generated connection id,
but I think it's not worth the hassle.

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 19, 2011

Well, maybe mark this somewhere in the issue tracker or the source as an potential issue. I doubt this is something worth wasting too much time on, but should be kept in mind. Basically if there are no ill side effects from this it's fine. If there are it definitively should get some considerations for work-arounds.

Anyway: A fix for this would also require awareness on the status query part which might not be that trivial as multiple information blocks might be returned for the same ID. This currently isn't handled at all and thus might cause major headaches ;-) Also: How would the client distinguish which of the "multi-result" returned belongs to which connection? Based on the size information? Based on the filename?

Okay: The issue exists and we just don't think about it any further. People who cause this are mad-minded :P

Well, maybe mark this somewhere in the issue tracker or the source as an potential issue. I doubt this is something worth wasting too much time on, but should be kept in mind. Basically if there are no ill side effects from this it's fine. If there are it definitively should get some considerations for work-arounds.

Anyway: A fix for this would also require awareness on the status query part which might not be that trivial as multiple information blocks might be returned for the same ID. This currently isn't handled at all and thus might cause major headaches ;-) Also: How would the client distinguish which of the "multi-result" returned belongs to which connection? Based on the size information? Based on the filename?

Okay: The issue exists and we just don't think about it any further. People who cause this are mad-minded :P

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

Seems to function but might be prone to format issues or buffer overflows. Please someone recheck this.

Seems to function but might be prone to format issues or buffer overflows. Please someone recheck this.

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 16, 2011

Owner

node->lenght's type is "apr_off_t", I trust apache's developers that APR_OFF_T_FMT is right format string to use

Owner

mpokrywka replied Jul 16, 2011

node->lenght's type is "apr_off_t", I trust apache's developers that APR_OFF_T_FMT is right format string to use

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 16, 2011

Well, I think too that they where knowing what they declared, I'm just not quite comfortable with the sscanf call itself. What about error conditions like someone putting 0xSomething in the Content-Length header? Or other stuff to throw the parsing off? IMHO there should be some checks if we got the proper data here.

Do we really have to check the Content-Length header here ourselves or is the information available in prepaired form somewhere?

Well, I think too that they where knowing what they declared, I'm just not quite comfortable with the sscanf call itself. What about error conditions like someone putting 0xSomething in the Content-Length header? Or other stuff to throw the parsing off? IMHO there should be some checks if we got the proper data here.

Do we really have to check the Content-Length header here ourselves or is the information available in prepaired form somewhere?

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 19, 2011

Owner

I see what you mean, I should use apr_strtoff + additional checks as in apache's http_filter.c
I checked that apache validates Content-Length header to be valid positive integer, I'll see if parsed value can be somehow extracted.

Owner

mpokrywka replied Jul 19, 2011

I see what you mean, I should use apr_strtoff + additional checks as in apache's http_filter.c
I checked that apache validates Content-Length header to be valid positive integer, I'll see if parsed value can be somehow extracted.

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 19, 2011

Thanks. Just do a pull request for the patch and I'm sure to merge it ASAP.

Thanks. Just do a pull request for the patch and I'm sure to merge it ASAP.

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

Local macro should be undefined after local use or defined globally.

Local macro should be undefined after local use or defined globally.

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 18, 2011

Owner

Yeah, this should be separate function...

Owner

mpokrywka replied Jul 18, 2011

Yeah, this should be separate function...

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

How does this guarantee that node changes are atomic/thread-safe? Doesn't really look sane to me. Please explain.

How does this guarantee that node changes are atomic/thread-safe? Doesn't really look sane to me. Please explain.

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 16, 2011

Owner

I believe each apache http request is processed by one process/thread, so there is no "atomic/thread-safe" problems when accessing/modifying request local data (for example request headers). "reqinfo" above is such local data, allocated for each request in "upload_progress_handle_request" (see lines 198-204 of this patch)

Owner

mpokrywka replied Jul 16, 2011

I believe each apache http request is processed by one process/thread, so there is no "atomic/thread-safe" problems when accessing/modifying request local data (for example request headers). "reqinfo" above is such local data, allocated for each request in "upload_progress_handle_request" (see lines 198-204 of this patch)

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 16, 2011

Okay, so basically we have a one-writer, multiple-readers situation here? If so this should be sane IMHO.

Okay, so basically we have a one-writer, multiple-readers situation here? If so this should be sane IMHO.

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 19, 2011

Owner

I don't know if you got the idea behind this whole commit, so there is more thorought explanation:

before this change, module worked like this:

  • upload data arrives -> apache finds shared memory with id -> updates shared memory
    after this change:
    • upload data arrives -> apache updates request local data
    • if shared memory was updated more than 500 ms ago -> find shared memory with id -> update shared memory

So there is more memory used - each request has its own local data, shared memory holds data copy.
No locking is neccesary when modifying request local data - as I mentioned each request has its own process/thread.
Accessing shared memory didn't change - is still protected by CACHE_LOCK.
Whole gain is at lines 347-355, cache lock is now only done twice per second,
earlier lock was acquired for each packet (~50/second for 512kbps upload)

Owner

mpokrywka replied Jul 19, 2011

I don't know if you got the idea behind this whole commit, so there is more thorought explanation:

before this change, module worked like this:

  • upload data arrives -> apache finds shared memory with id -> updates shared memory
    after this change:
    • upload data arrives -> apache updates request local data
    • if shared memory was updated more than 500 ms ago -> find shared memory with id -> update shared memory

So there is more memory used - each request has its own local data, shared memory holds data copy.
No locking is neccesary when modifying request local data - as I mentioned each request has its own process/thread.
Accessing shared memory didn't change - is still protected by CACHE_LOCK.
Whole gain is at lines 347-355, cache lock is now only done twice per second,
earlier lock was acquired for each packet (~50/second for 512kbps upload)

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 19, 2011

I guessed something like this and that's also why I accepted this change for the larger pull request I merged. Just wasn't sure if the locking was sufficient. So thanks for this explanation.

I guessed something like this and that's also why I accepted this change for the larger pull request I merged. Just wasn't sure if the locking was sufficient. So thanks for this explanation.

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

I don't quite like this commit (even though it is completely legal to do it) because of the source decentralization on node key checks.

The inline directive on the original check_node function had exactly the same effect even if the resulting code would have been a few instruction longer. Those additional instructions where to make the return value a bit more deterministic but would be optimized away by the compiler thus in actual usage of this function there'd be no difference.

Also keeping this function would allow for better sanity checking when retrieving keys (using the check_node functions for code assertions).

BenBE commented on 3193a18 Jul 15, 2011

I don't quite like this commit (even though it is completely legal to do it) because of the source decentralization on node key checks.

The inline directive on the original check_node function had exactly the same effect even if the resulting code would have been a few instruction longer. Those additional instructions where to make the return value a bit more deterministic but would be optimized away by the compiler thus in actual usage of this function there'd be no difference.

Also keeping this function would allow for better sanity checking when retrieving keys (using the check_node functions for code assertions).

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

Possible integer overflow due to undefined size of integer type.

I know this isn't a real issue on most platforms and this change makes the code less cast-y but shouldn't we use uint64_t or int64_t instead i.e. a type that's guaranteed to be of sufficient size (and be properly defined in its size)?

Possible integer overflow due to undefined size of integer type.

I know this isn't a real issue on most platforms and this change makes the code less cast-y but shouldn't we use uint64_t or int64_t instead i.e. a type that's guaranteed to be of sufficient size (and be properly defined in its size)?

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

Should be:
/* APR version 0.x does not support apr_shm_remove, so using a backport copy from APR version 1.x */

In addition the version of APR which this version was taken from should be added for documentation purposes.

Should be:
/* APR version 0.x does not support apr_shm_remove, so using a backport copy from APR version 1.x */

In addition the version of APR which this version was taken from should be added for documentation purposes.

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

For better style this should be wrapped differently to avoid splitting the format string:

ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
"Failed to attach to shared memory file '%s'",
config->cache_file);

For better style this should be wrapped differently to avoid splitting the format string:

ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
"Failed to attach to shared memory file '%s'",
config->cache_file);

@BenBE

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 15, 2011

if config->cache guaranteed to be non-NULL after this? If not: Ensure the later pointers are properly set up to allow for easy allocation checks or make initialization fail.

if config->cache guaranteed to be non-NULL after this? If not: Ensure the later pointers are properly set up to allow for easy allocation checks or make initialization fail.

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 18, 2011

Owner

If config->cache_shm is valid (result of apr_shm_create/apr_shm_attach), apr_shm_baseaddr_get will return valid pointer

Owner

mpokrywka replied Jul 18, 2011

If config->cache_shm is valid (result of apr_shm_create/apr_shm_attach), apr_shm_baseaddr_get will return valid pointer

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 19, 2011

What about putting a small note (or an assertion) here? I know I'm kinda paranoid here, but better safe than sorry (well, in case of failure the fail should be quite obvious if this was the cause ;-)).

What about putting a small note (or an assertion) here? I know I'm kinda paranoid here, but better safe than sorry (well, in case of failure the fail should be quite obvious if this was the cause ;-)).

This comment has been minimized.

Show comment Hide comment
@mpokrywka

mpokrywka Jul 20, 2011

Owner

I appreciate your concerns but apr_shm_baseaddr_get canot fail - I read the code, see:

srclib/apr/shmem/unix/shm.c

APR_DECLARE(void *) apr_shm_baseaddr_get(const apr_shm_t *m)
{
    return m->usable;
}

srclib/apr/shmem/win32/shm.c

APR_DECLARE(void *) apr_shm_baseaddr_get(const apr_shm_t *m)
{
    return m->usrmem;
}
Owner

mpokrywka replied Jul 20, 2011

I appreciate your concerns but apr_shm_baseaddr_get canot fail - I read the code, see:

srclib/apr/shmem/unix/shm.c

APR_DECLARE(void *) apr_shm_baseaddr_get(const apr_shm_t *m)
{
    return m->usable;
}

srclib/apr/shmem/win32/shm.c

APR_DECLARE(void *) apr_shm_baseaddr_get(const apr_shm_t *m)
{
    return m->usrmem;
}

This comment has been minimized.

Show comment Hide comment
@BenBE

BenBE Jul 21, 2011

That's sane then ;-)

That's sane then ;-)

BenBE added a commit that referenced this pull request Jul 15, 2011

Merge pull request #24 from mpokrywka/master
Lotsastuff: My earlier changes, BenBE's changes, julthomas' fixes

I've reviewed your patches and there are no obvious issues for which I'd reject them; though there are some minor tweaks that might be applied later.

There are some patches with lines I'm not fully sure about, so be sure to check things on the mentioned commits for further comments.

There was no immediate problem that would recommend not merging certain changes but be vary there are some some uncertainties involved which should be rechecked.

@BenBE BenBE merged commit a931beb into drogus:master Jul 15, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment